-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add SQLite caching with --no-cache and --force-check flags (#2219) #2608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add SQLite caching with --no-cache and --force-check flags (#2219) #2608
Conversation
Implements SQLite-based result caching to improve performance and reduce rate limiting. Results are cached for 24 hours by default and stored in ~/.sherlock/cache.db. Features: - Automatic caching of username lookup results with configurable TTL - --no-cache flag to disable caching completely - --force-check flag to ignore cached results and force fresh lookups - --cache-duration flag to customize cache expiration (default: 86400s) - sherlock-cache CLI utility for cache management (stats, clear, cleanup) - Comprehensive test suite for cache functionality Technical Details: - Cache stored in SQLite database at ~/.sherlock/cache.db - Automatic cleanup of expired entries on each run - Caches both CLAIMED and AVAILABLE status results - Thread-safe database operations - Zero dependencies (uses built-in sqlite3 module) Resolves sherlock-project#2219
This PR is part of Hacktoberfest 2025 contribution efforts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, have any checks been performed for the possibility of SQLi?
This was not a complete review, as that'll take some time -- just some comments at first glance.
[PEP 8] (second bullet point, primarily, with a blank line separator between sections and two lines after the final import)
Addresses all feedback from PR sherlock-project#2608 review by @ppfeister Security Hardening: - Implement SQL injection protection via parameterized queries in all database operations (get, set, clear, cleanup_expired, get_stats) - Add comprehensive input validation (null bytes, control characters, length limits) to prevent injection attacks - Implement path traversal protection restricting cache to ~/.sherlock - Add URL validation (max 2048 chars, no null bytes) - Store cache_duration per entry to prevent TTL drift across runs Code Quality (PEP 8 Compliance): - Fix import ordering: stdlib → third-party → local with blank line separators in cache.py, cache_cli.py, and sherlock.py - Replace Any type hints with specific unions (str|int, QueryStatus) - Remove shebang and __main__ block from cache_cli.py to prevent unsupported direct script execution Testing Improvements: - Replace file-based tests with unittest.mock (no disk I/O) - Remove time.sleep() calls, use mocked timestamps instead - Add security-specific tests (SQL injection, path traversal, null bytes) - Verify parameterized query usage in all database operations - Follow maintainer's testing patterns from feat/better_waf branch - Fix unused variable linting warnings (F841) Database Migration: - Add automatic schema migration for existing cache databases - Detect and handle old schema missing cache_duration column - Gracefully drop and recreate incompatible cache tables Platform Compatibility: - Verify Windows compatibility (Path.home() behavior documented) - Test Docker container build and execution - Confirm cross-platform path separator handling Test Results: - Linting: ✓ All checks passed - Cache tests: ✓ 14/14 passed - Docker build: ✓ Verified with act - Integration tests: 38/39 passed (1 flaky external site WAF)
✅ Fixed - All database operations now use parameterized queries with ? placeholders. Added comprehensive input validation (null bytes, control characters, length limits). Tests verify parameterized query usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attached a couple additional thoughts
Appreciate your patience and cooperation on this one. As a larger architectural change, I'd rather make sure it's right the first time rather than have things unexpectedly break.
[--site SITE_NAME] [--proxy PROXY_URL] [--json JSON_FILE] | ||
[--timeout TIMEOUT] [--print-all] [--print-found] [--no-color] | ||
[--browse] [--local] [--nsfw] | ||
[--browse] [--local] [--nsfw] [--no-cache] [--force-check] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do --skip-cache
and --ignore-cache
? I feel like that just removes some ambiguity.
Like, what does --force-check even do? Of course I want to check these usernames. Maybe it bypasses username validation? (of course we know what it does)
Open to hearing your thoughts.
if cache_path is None: | ||
cache_dir = Path.home() / ".sherlock" | ||
cache_path = str(cache_dir / "cache.db") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two items here...
Firstly, while .db is technically accurate, and extensions aren't important programmatically, it's always good to have proper hinting for the human factor. "What type of db is this random file?" Using .sqlite3
rather than .db
may be ideal.
And it adds some complexity (you'd have to check for OS type), but it occurs to me now that this is also non-standard.
Ideally, *nix type systems should see this cache at ~/.cache/sherlock/cache.sqlite3
(while not defined by the FHS, it is defined by XDG BDS and is otherwise convention). This is sometimes defined by $XDG_CACHE_HOME
, but many systems lack this variable (including my own, I'm now realizing) - but the above path is the expectation.
Similarly (while not a Windows user), I believe the convention there would expect %localappdata%\sherlock\cache.sqlite3
. This also prevents dirtying user home, or otherwise having to set NTFS file attrs to make the directory hidden (since dotfiles aren't automatically hidden as they are on *nix).
Again, open to thoughts/concerns/etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On that second point, the package platformdirs
may be useful. Would require a poetry add platformdirs
from platform dirs import user_cache_dir
and user_cache_dir(<appname>, <appauthor>)
I have not validated this functionality, just did a cursory search. Pretty sure I've used it in the past.
Using something like
from platformdirs import PlatformDirs
dirs = PlatformDirs("sherlock", "sherlock_project", version=__version__)
cache_dir = dirs.user_cache_dir
may be the most portable and alleviate most schema migration concerns, due to built in versioning by path
# Migration: Check if cache_duration column exists | ||
cursor.execute("PRAGMA table_info(results)") | ||
columns = [row[1] for row in cursor.fetchall()] | ||
|
||
if 'cache_duration' not in columns: | ||
# Add cache_duration column to existing table | ||
cursor.execute(''' | ||
ALTER TABLE results | ||
ADD COLUMN cache_duration INTEGER NOT NULL DEFAULT 86400 | ||
''') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we offload migration into a separate function?
Additionally, it's probably best to use PRAGMA user_version
(starting at 1) to track schema versions automatically, where an older schema version can be automatically migrated as appropriate (rather than arbitrarily checking for the existence of certain cols)
The function can take no arguments and doesn't need to return anything at this time, just migrate as needed, or allow an exception to be raised if there's a fatal error.
# Cache the result if enabled | ||
if cache and result.status in [QueryStatus.CLAIMED, QueryStatus.AVAILABLE]: | ||
cache.set( | ||
username=username, | ||
site=social_network, | ||
status=result.status, | ||
url=result.site_url_user if result.status == QueryStatus.CLAIMED else None | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this running on each username check rather than at the end lead to possible time-of-use issues?
Username 1 loads db state A
Username 2 loads db state A
Username 1 writes db state A->Z
Username 2 writes db state A->X, over top of Z
End result of db in state X, losing the changes made by Username 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if this is an issue..)
This doesn't necessarily have to be a post-run function either, which may fail to cache results after an early termination, it can be a parallelism-safe side function as well. But maybe failing to cache due to early termination is desirable. I can see arguments for both sides. I might lean towards do-not-cache at early termination and keeping it easy as a post-run fx, since most early terminations are bad runs anyhow
Additional nice-to-have ---- If we could configure cache settings by environment variable, that would be extremely useful in terms of automations and containerized environments i.e. (can't imagine a need for the others) Easy enough for this PR or should we break that out into a separate RFE? |
Description
Implements SQLite-based result caching as discussed in #2219 to improve performance and reduce rate limiting when performing multiple username lookups.
Changes
Core Caching Implementation
sherlock_project/cache.py
- SQLite cache manager with TTL supportsherlock_project/sherlock.py
- Integrated cache into main sherlock function~/.sherlock/cache.db
(automatic directory creation)CLI Arguments Added
--no-cache
- Disable caching completely (don't read or write to cache)--force-check
- Ignore cached results and force fresh checks for all sites--cache-duration SECONDS
- Customize cache expiration time (default: 86400)Cache Management Utility
sherlock-cache
with subcommands:stats
- Show cache statistics (total/valid/expired entries, cache path)clear
- Clear cache entries (all, by username, or by site)cleanup
- Remove only expired entriesTesting & Documentation
tests/test_cache.py
- Comprehensive cache functionality testsdocs/README.md
- Added cache usage and management sectionUsage Examples
Performance Impact
--no-cache
Implementation Details
Cache Strategy
CLAIMED
andAVAILABLE
status resultsUNKNOWN
,ILLEGAL
, orWAF
statuses(username, site)
tuple (prevents conflicts)Database Schema
Testing
Test Results: ✅ All 5 cache tests passing
test_cache_set_and_get
- Basic cache operationstest_cache_expiration
- TTL functionalitytest_cache_clear_all
- Clear entire cachetest_cache_clear_username
- Clear by usernametest_cache_stats
- Statistics generationRelated Issue
Closes #2219
Checklist
--no-cache
flag--force-check
flagsherlock-cache
management utilityCode of Conduct